Skip to content

Conversation

@jorgecarleitao
Copy link
Member

@jorgecarleitao jorgecarleitao commented Dec 26, 2020

This PR removes an unused trait and associated API that returns Result<()> for infalible implementations

Both of these are historical artifacts derived from using io::write, which was abandoned because these operations are infalible.

Copy link
Member Author

@jorgecarleitao jorgecarleitao Dec 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was motivated by T not really requiring ArrowPrimitiveType, only ArrowNativeType, and is required for a major simplification on the Buillders, #9019

@github-actions
Copy link

@codecov-io
Copy link

codecov-io commented Dec 26, 2020

Codecov Report

Merging #9017 (bde093f) into master (a4f7c4a) will decrease coverage by 0.00%.
The diff coverage is 92.80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9017      +/-   ##
==========================================
- Coverage   82.87%   82.87%   -0.01%     
==========================================
  Files         201      201              
  Lines       49739    49733       -6     
==========================================
- Hits        41220    41215       -5     
+ Misses       8519     8518       -1     
Impacted Files Coverage Δ
rust/arrow/src/array/builder.rs 84.05% <90.42%> (-0.07%) ⬇️
rust/arrow/src/compute/kernels/comparison.rs 96.28% <100.00%> (ø)
rust/arrow/src/tensor.rs 87.50% <100.00%> (ø)
rust/parquet/src/arrow/array_reader.rs 75.49% <100.00%> (ø)
rust/parquet/src/arrow/record_reader.rs 96.25% <100.00%> (ø)
rust/parquet/src/encodings/encoding.rs 95.43% <0.00%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4f7c4a...bde093f. Read the comment docs.

Copy link
Contributor

@Dandandan Dandandan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, LGTM 👍 I believe there are also quite some other places returning unneeded Results.

/// and resizes the buffer as needed.
///
/// The values of the newly added elements are undefined.
/// The values of the newly added elements are 0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe T::default()

pub type UInt64BufferBuilder = BufferBuilder<UInt64Type>;
pub type Float32BufferBuilder = BufferBuilder<Float32Type>;
pub type Float64BufferBuilder = BufferBuilder<Float64Type>;
pub type Int8BufferBuilder = BufferBuilder<i8>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 This is a nice simplification

@alamb
Copy link
Contributor

alamb commented Dec 27, 2020

I also merged from master and ran the tests locally to make sure this didn't logically conflict. Everything was good to go!

@alamb alamb closed this in 48676f2 Dec 27, 2020
@alamb
Copy link
Contributor

alamb commented Dec 27, 2020

This is a nice cleanup -- thank you @jorgecarleitao

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants